fix(metrics): never emit runtime_http_* samples without a handler label by silvadenisaraujo · Pull Request #673 · vtex/node-vtex-api

silvadenisaraujo · 2026-08-03T17:40:28Z

Problem

After /metrics started serving the cluster-wide aggregate (#667, @vtex/api@7.4.1, service-node:7.7.14), dashboards grew an extra, nameless line:

runtime_http_requests_total{handler="builtin:healthcheck",status_code="200"} 471
runtime_http_requests_total{handler="private-handler:ssr",status_code="200"} 421
runtime_http_requests_total{status_code="200"} 21   ← no handler label at all
runtime_http_requests_total{status_code="404"} 2    ← no handler label at all

Observed live on vtex-render-ssr in prod-dj-ioadmin-eks-use1a-t1d / vendor-vtex.

Root cause

ctx.requestHandlerName is only assigned inside a route pipeline (nameSpanOperationMiddleware) or by a builtin handler, but addRequestMetricsMiddleware is mounted at the top of the chain (worker/index.ts:245) and counts every request in a finally block. So requests that never reach a named handler are counted with handler: undefined.

prom-client keeps the key in memory, so the local exposition rendered it as handler="undefined". Node's cluster IPC serializes messages as JSON, and JSON.stringify drops undefined values, so the sample reaches the master with the handler key gone. Verified against the pinned prom-client@14.2.0:

stored labels object keys: [ 'handler', 'status_code' ]  has handler prop: true  value: undefined

local registry (pre-aggregation / workers===1):  m_total{handler="undefined",status_code="200"} 2
aggregate WITHOUT ipc serialization:             m_total{handler="undefined",status_code="200"} 2
aggregate AFTER  ipc serialization (real path):  m_total{status_code="200"} 2      ← label stripped

Prometheus reads an absent label as handler="", so:

it is a different series from the historical handler="undefined" → panels split at the rollout boundary;
Grafana has no value to interpolate into {{handler}} and falls back to the default field name Value;
exclusion filters stop working — handler!~"builtin:.*|undefined" does not match "", so the bucket that used to be filtered out is now included.

Which requests are affected

source	status	note
`GET /_status` (platform status poller)	200	reaches `statusTrackHandler`, which sets only the span name, never `ctx.requestHandlerName`
unmatched paths	404	no pvt route matches and no `x-colossus-route-id` → chain ends, Koa answers its default 404
replica-level rate limit	429	`concurrentRateLimiter` is mounted before the routers and throws
unimplemented / unknown route id	501 / 404 / 400	`routerFromPublicHttpHandlers`, `routerFromEventHandlers` return before naming
aborted requests	—	`abortedRequests.inc` uses the same undefined name

Reproduced live: 5× GET /_status moved {status_code="200"} by exactly +5 (+1 background poll); two requests to unmatched paths created {status_code="404"} 2; HEAD /healthcheck and GET /_metrics stayed correctly labelled as builtin:healthcheck / builtin:metrics-logger.

Proposal

src/service/metrics/requestHandlerLabel.ts (new) — one place that resolves the label, falling back to 'undefined', with the reasoning documented next to the constant.

'undefined' rather than a nicer word like 'unnamed' is deliberate: it is exactly what prom-client rendered locally before cluster aggregation existed, so the aggregated output keeps the historical series identity and dashboards/alerts already filtering on handler="undefined" (e.g. handler!~"builtin:.*|undefined") keep working with no query changes. Empty strings fall back too, so the label is never emitted empty.
requestMetricsMiddleware.ts / otelRequestMetricsMiddleware.ts — use the helper at all four call sites each (total, aborted, response sizes, timings). Evaluation stays inside the callbacks/finally, so the handler name is still read after the pipeline ran.
statusTrack.ts — set ctx.requestHandlerName = 'builtin:status-track', parity with the three sibling builtins. /_status traffic gets its own series instead of polluting the catch-all bucket. This commit is separable if reviewers prefer to ship only (1)+(2) — note /_status is in PATHS_BLACKLISTED_FOR_TRACING, so the pre-existing setOperationName call is usually a no-op, which is likely why the missing assignment went unnoticed.

No metric names, help text, buckets or label names change.

Tests

src/service/metrics/__tests__/requestHandlerLabel.test.ts (new) — drives the real middleware and asserts the label survives a cluster IPC JSON round-trip, that no sample is emitted with a missing/empty handler, that named and unnamed handlers stay separate series, and that aborted requests are labelled. Reverting the fallback makes 5 of these 7 cases fail.
src/service/metrics/__tests__/clusterMetricsAggregator.test.ts — the aggregation helper now round-trips worker registries through JSON, so these tests exercise what the master actually receives. The absence of that round-trip is why Aggregate prom-client metrics across cluster workers for /metrics #667 didn't catch this.
src/service/worker/runtime/__tests__/statusTrack.test.ts (new) — asserts /_status names itself, with and without tracing.

jest: 17 suites, 239 passed (24 pre-existing skips). tsc --noEmit clean. tslint reports no new findings.

Rollout note

The unnamed series (handler="") exists only on runtimes carrying #667 without this fix, i.e. service-node:7.7.14 up to the release that includes this PR. Dashboards looking back across that window can stitch the two shapes with:

sum by (handler) (
  label_replace(
    rate(runtime_http_requests_total{cluster=~"$cluster", app="$app_name"}[$__rate_interval]),
    "handler", "undefined", "handler", "^$"
  )
)

Do the label_replace inside the aggregation, otherwise the relabelled series can collide with a real handler="undefined" series during the rollout and Prometheus errors with vector cannot contain metrics with the same labelset.

Requests that never reach a named handler (unmatched paths answered by Koa's default 404, rejections by the replica-level rate limiter, errors thrown before the route pipeline) were counted with `handler: undefined`, because `ctx.requestHandlerName` is only assigned inside a route pipeline while addRequestMetricsMiddleware counts every request in a `finally` block. prom-client keeps the label key in memory, so the local exposition rendered it as `handler="undefined"`. Node's cluster IPC serializes each worker's registry as JSON, and JSON.stringify drops properties whose value is `undefined`, so once /metrics started serving the cluster aggregate those samples arrived at the master without the `handler` key at all. Prometheus reads an absent label as `handler=""`, producing a second, unnamed series that dashboards render as a nameless "Value" line and that filters such as `handler!~"builtin:.*|undefined"` no longer exclude. Resolve the label through a single helper that falls back to `"undefined"` — the value prom-client already rendered locally — so the aggregated output keeps the historical series identity and existing dashboards and alerts keep working. The same fallback is applied to the OpenTelemetry request instruments. The aggregation tests now round-trip worker registries through JSON, reproducing what the master really receives; without the fallback five of the new cases fail.

statusTrackHandler answers 200 (it assigns `ctx.body`), so its requests do reach a handler — it just never set `ctx.requestHandlerName`, unlike healthcheck, whoami and metrics-logger, which set both the request handler name and the span operation name. Its samples therefore landed in the catch-all unnamed bucket. Set `ctx.requestHandlerName` for parity, which also makes the existing setOperationName call meaningful for callers that keep tracing enabled (/_status is in PATHS_BLACKLISTED_FOR_TRACING, so the span is usually absent).

…r-label fix to 6.x Backports two related master-line metrics changes onto the 6.x maintenance line as a single PR, so 6.x jumps straight to the correct end state: 1. Cluster-wide /metrics aggregation (PR #667). In multi-worker mode the worker answering a scrape asks the master for a merged, monotonic view built from every worker's registry over the existing cluster IPC (prom-client AggregatorRegistry), with a bounded timeout and local-registry fallback. Single-worker mode (workers === 1, incl. LINKED) is unchanged. New module src/service/metrics/clusterMetricsAggregator.ts owns the message constants, guards, master-side handler and worker-side request fn. master/worker onMessage handlers now route the new messages and silently ignore prom-client's own getMetricsReq/getMetricsRes IPC messages. 2. Never emit runtime_http_* samples without a handler label (PR #673). New src/service/metrics/requestHandlerLabel.ts resolves the label with an explicit 'undefined' fallback (deliberately that exact string, to preserve historical series identity through the cluster-IPC JSON round-trip that drops undefined). Used at all requestMetricsMiddleware call sites; statusTrackHandler sets ctx.requestHandlerName = 'builtin:status-track' for parity with sibling builtins. Skips the otel middleware slice of #673 (absent on 6.x). prom-client unchanged. Bumps version 6.51.0 -> 6.52.0 and adds a CHANGELOG entry. jest.config.js: add a moduleNameMapper for OpenTelemetry's otlp-exporter-base/node-http subpath export so the new metrics suites (and the pre-existing rateLimit suite) load under jest@25, whose resolver predates the package "exports" field.

juliobguedes · 2026-08-27T17:52:29Z

Ignoring SonarQube as its comments are not related to this change, but part of our technical debts in the backlog.

…ed-handler-label

dk-pr-review

DK Review — Audit Summary

Verdict: ❌ BLOCKED

Severity	Count
BLOCK	1
RESTRICT	1
SUGGEST	7

Scenarios evaluated: general-review, quality-ratchet
Scenarios skipped: dependency-governance (no matching files), pipeline-config (no matching files), agent-skills-review (no matching files)

📋 Findings (9)

BLOCK

[Functional.Resource] src/service/worker/runtime/__tests__/statusTrack.test.ts:15 — The new test calls statusTrackHandler without stubbing process.send. LINKED is !!process.env.VTEX_APP_LINK, which is false under Jest, so the handler executes process.send?.('broadcastStatusTrack'). Jest runs test files in jest-worker child processes that have a live IPC channel, so this sends a raw string message to the Jest parent; jest-worker's _onMessage switches on response[0] and throws TypeError: Unexpected response from worker: b for unrecognised payloads, which can abort the test run. Both it blocks trigger it. The sibling test file src/service/metrics/__tests__/clusterMetricsAggregator.test.ts already guards against exactly this by assigning and then delete (process as any).send in afterEach.
→ Stub the IPC channel in the test, e.g. beforeEach(() => { (process as any).send = jest.fn() }) and afterEach(() => { delete (process as any).send }), following the pattern already used in clusterMetricsAggregator.test.ts. Asserting that the broadcast was sent would also cover the !LINKED branch that is currently exercised by accident.

RESTRICT

[Functional.Resource] src/service/worker/runtime/__tests__/statusTrack.test.ts:15 — The new test calls statusTrackHandler without stubbing process.send. The handler runs process.send?.(BROADCAST_STATUS_TRACK) whenever LINKED is false (the default in CI), and Jest executes test files inside jest-worker child processes where process.send is a real IPC channel. The test therefore emits the raw string 'broadcastStatusTrack' onto Jest's worker protocol channel on every run — an uncontrolled side effect that can surface as worker protocol noise or flaky runs, and the broadcast branch is asserted nowhere.
→ Stub the channel in the test (e.g. const send = jest.fn(); (process as any).send = send) and restore it in afterEach, then assert the broadcast behaviour explicitly for both LINKED states instead of letting the real IPC call escape.

SUGGEST

[general.documentation-gap] CHANGELOG.md:10 — The changelog entry scopes the fix to runtime_http_* Prometheus metrics and justifies it entirely with Node cluster IPC JSON serialization, but requestHandlerLabel was also applied to otelRequestMetricsMiddleware.ts (aborted requests, response sizes, total requests, request timings). OpenTelemetry metrics do not go through the cluster IPC round-trip; for that exporter this silently changes the handler attribute from absent/undefined to the literal string "undefined", which is a series-identity change for any OTel-backed dashboard or alert. That impact is undocumented.
→ Add a changelog bullet covering the OpenTelemetry request metrics as well, stating that the handler attribute is now always present and is "undefined" for unnamed handlers, so consumers of the OTel pipeline can adjust queries.
[Evolvability.Organizational] src/service/metrics/__tests__/requestHandlerLabel.test.ts:9 — The overClusterIpc helper and its explanatory comment are duplicated verbatim between requestHandlerLabel.test.ts and clusterMetricsAggregator.test.ts. Since this helper encodes the central assumption of the whole fix (cluster IPC drops undefined label values), having two copies means a future correction to that assumption can be applied to only one of them.
→ Extract overClusterIpc into a shared test helper (e.g. src/service/metrics/__tests__/helpers/clusterIpc.ts) and import it from both test files.
[Functional.Check] src/service/metrics/otelRequestMetricsMiddleware.ts:43 — otelRequestMetricsMiddleware.ts is changed in four places but has no test coverage in this PR — the new requestHandlerLabel.test.ts exercises only addRequestMetricsMiddleware (prom-client). A regression that drops the fallback on the OTel path (or a future refactor of the attribute objects) would not be caught.
→ Add at least one case driving addOtelRequestMetricsMiddleware with a stubbed getOtelInstruments, asserting that every recorded instrument receives a non-empty handler attribute for a ctx with requestHandlerName === undefined.
[general.documentation-gap] CHANGELOG.md:21 — The new ### Fixed block sits directly above ## [7.4.0] - 2026-06-22, but package.json declares version 7.4.2. Releases 7.4.1 and 7.4.2 have no changelog entries, so the Unreleased section is being appended to a changelog that is already two patch versions behind the published package — anyone cutting a release from this file will produce misleading release notes.
→ Ask the author to confirm and backfill the missing [7.4.1] and [7.4.2] sections (or explain why they were intentionally omitted) before this Unreleased block is promoted to a version heading.
[quality.new-logic-enforcement] src/service/metrics/otelRequestMetricsMiddleware.ts:42 — addOtelRequestMetricsMiddleware gains the same requestHandlerLabel(...) fallback in four places (aborted, response sizes, total requests, timings), but no test in this PR exercises the OTel middleware. The new suite requestHandlerLabel.test.ts only covers the prom-client addRequestMetricsMiddleware, so the OTel branch of the fix — including the aborted-request path — ships without coverage and could silently regress.
→ Add a test that drives addOtelRequestMetricsMiddleware with a mocked getOtelInstruments() and asserts the handler attribute equals 'undefined' when ctx.requestHandlerName is unset, mirroring the prom-client cases.
[Functional.Check] src/service/metrics/__tests__/requestHandlerLabel.test.ts:97 — The 'emits no sample with a missing or empty handler label' test nests its only assertion inside handlerLabelledMetrics.forEach(... samplesOf(...).forEach(...)). If a metric name drifts (e.g. runtime_http_response_size_bytes is renamed) or a sample stops being emitted, samplesOf returns an empty array, the inner forEach never runs, and the test passes vacuously — exactly the regression it is meant to guard against would go undetected.
→ Assert the sample list is non-empty before iterating, e.g. const samples = samplesOf(aggregated, metric); expect(samples.length).toBeGreaterThan(0); samples.forEach(...).
[Evolvability.Organizational] src/service/metrics/__tests__/requestHandlerLabel.test.ts:9 — The overClusterIpc helper and its explanatory comment are duplicated verbatim in requestHandlerLabel.test.ts and clusterMetricsAggregator.test.ts. Since this helper encodes a subtle, load-bearing assumption about Node cluster IPC JSON serialization, two independent copies will drift and one may silently stop reproducing the real transport.
→ Extract overClusterIpc into a shared test helper (e.g. src/service/metrics/__tests__/helpers/clusterIpc.ts) and import it from both suites so the IPC-fidelity assumption is defined once.

DK Review v1.0.0 | To dismiss a finding: reply /dk-review dismiss <finding-id> [reason]

dk-pr-review · 2026-08-27T18:27:57Z

+    }
+
+    await statusTrackHandler(ctx as ServiceContext)
+


[Functional.Resource] 🔴 BLOCK

The new test calls statusTrackHandler without stubbing process.send. LINKED is !!process.env.VTEX_APP_LINK, which is false under Jest, so the handler executes process.send?.('broadcastStatusTrack'). Jest runs test files in jest-worker child processes that have a live IPC channel, so this sends a raw string message to the Jest parent; jest-worker's _onMessage switches on response[0] and throws TypeError: Unexpected response from worker: b for unrecognised payloads, which can abort the test run. Both it blocks trigger it. The sibling test file src/service/metrics/__tests__/clusterMetricsAggregator.test.ts already guards against exactly this by assigning and then delete (process as any).send in afterEach.

Action: Stub the IPC channel in the test, e.g. beforeEach(() => { (process as any).send = jest.fn() }) and afterEach(() => { delete (process as any).send }), following the pattern already used in clusterMetricsAggregator.test.ts. Asserting that the broadcast was sent would also cover the !LINKED branch that is currently exercised by accident.

To dismiss: /dk-review dismiss f3b1c2a7-5d84-4e19-9c07-2a6e8b41d0f5 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

 ## [Unreleased]
+### Fixed
+- `runtime_http_*` metrics no longer emit samples without a `handler` label. Requests
+  that never reach a named handler (unmatched paths, replica-level rate limit


[general.documentation-gap] 🔵 SUGGEST

The changelog entry scopes the fix to runtime_http_* Prometheus metrics and justifies it entirely with Node cluster IPC JSON serialization, but requestHandlerLabel was also applied to otelRequestMetricsMiddleware.ts (aborted requests, response sizes, total requests, request timings). OpenTelemetry metrics do not go through the cluster IPC round-trip; for that exporter this silently changes the handler attribute from absent/undefined to the literal string "undefined", which is a series-identity change for any OTel-backed dashboard or alert. That impact is undocumented.

Action: Add a changelog bullet covering the OpenTelemetry request metrics as well, stating that the handler attribute is now always present and is "undefined" for unnamed handlers, so consumers of the OTel pipeline can adjust queries.

To dismiss: /dk-review dismiss 9a4d7e21-c3f6-4b58-8e0a-71d5c9f2b6e3 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

+// Node's cluster IPC serializes messages as JSON, which drops properties whose
+// value is `undefined`. This is what the master receives from each worker.
+const overClusterIpc = <T>(payload: T): T => JSON.parse(JSON.stringify(payload))
+


[Evolvability.Organizational] 🔵 SUGGEST

The overClusterIpc helper and its explanatory comment are duplicated verbatim between requestHandlerLabel.test.ts and clusterMetricsAggregator.test.ts. Since this helper encodes the central assumption of the whole fix (cluster IPC drops undefined label values), having two copies means a future correction to that assumption can be applied to only one of them.

Action: Extract overClusterIpc into a shared test helper (e.g. src/service/metrics/__tests__/helpers/clusterIpc.ts) and import it from both test files.

To dismiss: /dk-review dismiss 2c8f5b90-6a17-4d3e-b42c-0f9e7a1d84b6 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

-        instruments.abortedRequests.add(1, { [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName })
+        instruments.abortedRequests.add(1, {
+          [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName),
+        })


[Functional.Check] 🔵 SUGGEST

otelRequestMetricsMiddleware.ts is changed in four places but has no test coverage in this PR — the new requestHandlerLabel.test.ts exercises only addRequestMetricsMiddleware (prom-client). A regression that drops the fallback on the OTel path (or a future refactor of the attribute objects) would not be caught.

Action: Add at least one case driving addOtelRequestMetricsMiddleware with a stubbed getOtelInstruments, asserting that every recorded instrument receives a non-empty handler attribute for a ctx with requestHandlerName === undefined.

To dismiss: /dk-review dismiss 7d0e6431-b9c2-45a8-9f13-6e2b8c5a0f47 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

+  the other builtin handlers, instead of falling into the unnamed bucket.

 ## [7.4.0] - 2026-06-22
 ### Changed


[general.documentation-gap] 🔵 SUGGEST

The new ### Fixed block sits directly above ## [7.4.0] - 2026-06-22, but package.json declares version 7.4.2. Releases 7.4.1 and 7.4.2 have no changelog entries, so the Unreleased section is being appended to a changelog that is already two patch versions behind the published package — anyone cutting a release from this file will produce misleading release notes.

Action: Ask the author to confirm and backfill the missing [7.4.1] and [7.4.2] sections (or explain why they were intentionally omitted) before this Unreleased block is promoted to a version heading.

To dismiss: /dk-review dismiss b5417cae-2f68-4903-a7de-31c0d9b6e825 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

+    }
+
+    await statusTrackHandler(ctx as ServiceContext)
+


[Functional.Resource] 🟡 RESTRICT

The new test calls statusTrackHandler without stubbing process.send. The handler runs process.send?.(BROADCAST_STATUS_TRACK) whenever LINKED is false (the default in CI), and Jest executes test files inside jest-worker child processes where process.send is a real IPC channel. The test therefore emits the raw string 'broadcastStatusTrack' onto Jest's worker protocol channel on every run — an uncontrolled side effect that can surface as worker protocol noise or flaky runs, and the broadcast branch is asserted nowhere.

Action: Stub the channel in the test (e.g. const send = jest.fn(); (process as any).send = send) and restore it in afterEach, then assert the broadcast behaviour explicitly for both LINKED states instead of letting the real IPC call escape.

To dismiss: /dk-review dismiss 3f2a91c4-6d18-4b7e-9c05-1a8e2f7d4b31 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

      if (instruments) {
-        instruments.abortedRequests.add(1, { [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName })
+        instruments.abortedRequests.add(1, {
+          [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName),


[quality.new-logic-enforcement] 🔵 SUGGEST

addOtelRequestMetricsMiddleware gains the same requestHandlerLabel(...) fallback in four places (aborted, response sizes, total requests, timings), but no test in this PR exercises the OTel middleware. The new suite requestHandlerLabel.test.ts only covers the prom-client addRequestMetricsMiddleware, so the OTel branch of the fix — including the aborted-request path — ships without coverage and could silently regress.

Action: Add a test that drives addOtelRequestMetricsMiddleware with a mocked getOtelInstruments() and asserts the handler attribute equals 'undefined' when ctx.requestHandlerName is unset, mirroring the prom-client cases.

To dismiss: /dk-review dismiss b7c04e58-2f93-4a61-8de2-5c9b1a03f742 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

+
+    handlerLabelledMetrics.forEach((metric) => {
+      samplesOf(aggregated, metric).forEach((sample) => {
+        expect(sample).toMatch(/handler="[^"]+"/)


[Functional.Check] 🔵 SUGGEST

The 'emits no sample with a missing or empty handler label' test nests its only assertion inside handlerLabelledMetrics.forEach(... samplesOf(...).forEach(...)). If a metric name drifts (e.g. runtime_http_response_size_bytes is renamed) or a sample stops being emitted, samplesOf returns an empty array, the inner forEach never runs, and the test passes vacuously — exactly the regression it is meant to guard against would go undetected.

Action: Assert the sample list is non-empty before iterating, e.g. const samples = samplesOf(aggregated, metric); expect(samples.length).toBeGreaterThan(0); samples.forEach(...).

To dismiss: /dk-review dismiss d1e6b230-8a47-4c9f-b0d3-6e2f5c81a904 [reason]

dk-pr-review · 2026-08-27T18:27:58Z

+// Node's cluster IPC serializes messages as JSON, which drops properties whose
+// value is `undefined`. This is what the master receives from each worker.
+const overClusterIpc = <T>(payload: T): T => JSON.parse(JSON.stringify(payload))
+


[Evolvability.Organizational] 🔵 SUGGEST

The overClusterIpc helper and its explanatory comment are duplicated verbatim in requestHandlerLabel.test.ts and clusterMetricsAggregator.test.ts. Since this helper encodes a subtle, load-bearing assumption about Node cluster IPC JSON serialization, two independent copies will drift and one may silently stop reproducing the real transport.

Action: Extract overClusterIpc into a shared test helper (e.g. src/service/metrics/__tests__/helpers/clusterIpc.ts) and import it from both suites so the IPC-fidelity assumption is defined once.

To dismiss: /dk-review dismiss 9a58cf17-4b62-4e30-8f1c-7d34e6b902a5 [reason]

sonar-workflows · 2026-08-27T18:35:41Z

Quality Gate passed

Issues

Measures

Project ID: node-vtex-api

View in SonarQube

dk-pr-review

DK Review — Audit Summary

Verdict: ❌ BLOCKED

Severity	Count
BLOCK	1
RESTRICT	2
SUGGEST	10

Scenarios evaluated: dependency-governance, general-review, quality-ratchet
Scenarios skipped: pipeline-config (no matching files), agent-skills-review (no matching files)

📋 Findings (13)

BLOCK

[Functional.Resource] src/service/worker/runtime/__tests__/statusTrack.test.ts:15 — The new test invokes statusTrackHandler without stubbing process.send. LINKED is !!process.env.VTEX_APP_LINK, which is false under Jest, so the handler executes process.send?.('broadcastStatusTrack') for real. When Jest runs this file in a child-process worker (the default whenever more than one test file runs, including yarn ci:test), process.send is the jest-worker IPC channel; jest-worker's parent _onMessage switches on response[0] and throws TypeError: Unexpected response from worker: b for an unrecognized string message, aborting the run. Both tests in the file trigger this.
→ Stub the IPC channel in the test, following the convention already used in src/service/metrics/__tests__/clusterMetricsAggregator.test.ts: set (process as any).send = jest.fn() in beforeEach and delete (process as any).send in afterEach. Assert the broadcast while you are there — expect(sendMock).toHaveBeenCalledWith('broadcastStatusTrack') — so the side effect is covered rather than merely leaked.

RESTRICT

[Functional.Interface] src/service/metrics/otelRequestMetricsMiddleware.ts:43 — The handler="undefined" fallback is also applied to the OpenTelemetry instruments, but the justification documented in requestHandlerLabel.ts (Node cluster IPC serializes the prom-client worker registry as JSON and drops undefined values) only holds for the prom-client cluster aggregation path. OTel instruments export per-process and never traverse the cluster IPC JSON round-trip, so this changes the attribute set — and therefore the time-series identity — of the existing diagnostics abortedRequests/responseSizes/totalRequests/requestTimings series from "handler attribute absent" to handler="undefined". The CHANGELOG documents the change only for runtime_http_*, so consumers of the OTel/diagnostics metrics get an undocumented breaking change to their dashboards and alerts at the 7.5.0 boundary.
→ Either state explicitly in the CHANGELOG that the diagnostics/OTel handler attribute also changes from absent to undefined (and why consistency with the Prometheus series is desired), or keep the OTel call sites unchanged if the diagnostics backend already renders the missing attribute in a way existing dashboards depend on.
[Functional.Check] src/service/metrics/__tests__/requestHandlerLabel.test.ts:97 — The assertion in 'emits no sample with a missing or empty handler label' is vacuous: samplesOf(aggregated, metric).forEach(...) runs zero assertions when a metric produces no samples, so the test passes if the fix regresses to the point where the metric disappears from the aggregated output altogether. That is precisely the failure mode this PR is guarding against (a label/series vanishing through the cluster IPC round-trip).
→ Assert the sample set is non-empty before iterating, e.g. const samples = samplesOf(aggregated, metric); expect(samples.length).toBeGreaterThan(0); samples.forEach(...), or use expect.hasAssertions() plus an explicit expected-series list per metric.

SUGGEST

[Functional.Check] src/service/metrics/__tests__/requestHandlerLabel.test.ts:5 — The new regression suite only exercises addRequestMetricsMiddleware (prom-client). otelRequestMetricsMiddleware.ts received the same four-call-site change in this PR and remains completely untested, so a future revert or a missed call site there would not be caught by CI.
→ Add an equivalent test for addOtelRequestMetricsMiddleware with stubbed instruments, asserting that RequestsMetricLabels.REQUEST_HANDLER is always a non-empty string for aborted, sized, counted and timed requests.
[Evolvability.SolutionApproach] src/service/metrics/__tests__/requestHandlerLabel.test.ts:97 — The emits no sample with a missing or empty handler label test asserts inside a nested forEach, so it passes vacuously whenever samplesOf returns an empty array — e.g. if a metric is renamed, if the histogram never observes (response length falsy, close never emitted), or if register.clear() wipes an instrument the test expected. A regression that stops emitting these series altogether would be reported as green.
→ Assert expect(samples.length).toBeGreaterThan(0) for each metric before iterating, so the test fails when the expected samples are absent rather than silently passing.
[Evolvability.Organizational] src/service/metrics/__tests__/requestHandlerLabel.test.ts:9 — overClusterIpc is defined identically (implementation plus explanatory comment) in both requestHandlerLabel.test.ts and clusterMetricsAggregator.test.ts. Since it encodes a non-obvious invariant about the cluster IPC JSON round-trip that both suites depend on, duplicating it means a future correction has to be found and applied twice.
→ Extract overClusterIpc (with its comment) into a shared test helper under src/service/metrics/__tests__/ and import it from both suites.
[Evolvability.SupportedByLanguage] src/service/metrics/__tests__/requestHandlerLabel.test.ts:30 — runRequest(middleware: any, ctx: any) and the untyped object returned by buildCtx opt the whole suite out of type checking against ServiceContext and the middleware signature. The tests are specifically about a context property (ctx.requestHandlerName), so if that property is renamed or the middleware signature changes, these tests will compile cleanly and fail only at runtime — or, worse, keep passing against a stale shape.
→ Type buildCtx as Partial<ServiceContext> cast once at the boundary and give runRequest the real middleware type ((ctx: ServiceContext, next: () => Promise<void>) => Promise<void>), keeping the any cast confined to the single stub construction.
[general.clarity] CHANGELOG.md:8 — The new ## [7.5.0] heading has no release date, unlike every other entry in the file (## [7.4.0] - 2026-06-22), and the ## [Unreleased] section was removed rather than kept above the release. Both break the Keep a Changelog format this file declares it follows, and dropping [Unreleased] leaves the next contributor with no section to append to.
→ Write the heading as ## [7.5.0] - YYYY-MM-DD with the release date, and re-add an empty ## [Unreleased] section above it.
[general.documentation-gap] package.json:3 — The version goes 7.4.2 → 7.5.0 but the CHANGELOG has no entries for 7.4.1 or 7.4.2 — the newly released section is the former [Unreleased] block, so the cluster identity feature it documents under ### Added may in fact have shipped in one of those unrecorded patch releases. As written, the changelog attributes previously-shipped work to 7.5.0 and hides two releases entirely.
→ Confirm what shipped in 7.4.1/7.4.2, add the missing sections for them, and move any already-released item out of the 7.5.0 block so the version history is accurate for consumers pinning versions.
[general.broken-references] src/service/metrics/requestHandlerLabel.ts:17 — The whole design decision — choosing the literal string 'undefined' over a clearer value such as 'unnamed' — rests on two unverifiable external claims in the doc comment: that prom-client's local exposition historically rendered handler: undefined as handler="undefined", and that existing dashboards/alerts filter on it (handler!~"builtin:.*|undefined"). Neither can be confirmed from this repository.
→ Ask the author to double-check both references — the prom-client behaviour for the pinned version and at least one real dashboard/alert using that filter — and cite them (dashboard URL or alert rule) in the comment so a future reader can safely change the value.
[quality.new-logic-enforcement] src/service/metrics/otelRequestMetricsMiddleware.ts:42 — otelRequestMetricsMiddleware.ts gains the same requestHandlerLabel(...) fallback at four instrument call sites (abortedRequests, responseSizes, totalRequests, requestTimings) but no test accompanies it. The new requestHandlerLabel.test.ts covers only the prom-client path via addRequestMetricsMiddleware; the OTel middleware's attribute handling stays uncovered, so a future call site added without the helper would not be caught.
→ Add a test for addOtelRequestMetricsMiddleware with a mocked getOtelInstruments that asserts every recorded attribute set carries handler: 'undefined' when ctx.requestHandlerName is unset, mirroring the prom-client tests.
[quality.new-logic-enforcement] src/service/metrics/__tests__/requestHandlerLabel.test.ts:18 — buildCtx fixes requestHandlerName at context-construction time, so every test reads a handler name that already exists before the middleware runs. Real code — including statusTrack.ts in this very PR — assigns ctx.requestHandlerName during next(), and the middleware reads it afterwards in its finally block. That late-assignment ordering, the behaviour the statusTrack change depends on, is never exercised.
→ Add a case where next() sets ctx.requestHandlerName = 'builtin:status-track' before emitting close, and assert the emitted series is handler="builtin:status-track" rather than handler="undefined".
[Evolvability.Organizational] src/service/metrics/__tests__/clusterMetricsAggregator.test.ts:55 — overClusterIpc is defined verbatim in two test files (clusterMetricsAggregator.test.ts line 55 and requestHandlerLabel.test.ts line 9), each with its own copy of the explanatory comment. Duplicated test infrastructure drifts: the two comments already differ in wording, and a future correction to the IPC-fidelity model has to be applied twice.
→ Extract the helper into a shared test utility (e.g. src/service/metrics/__tests__/helpers/overClusterIpc.ts) and import it from both files, keeping a single authoritative comment.

DK Review v1.0.0 | To dismiss a finding: reply /dk-review dismiss <finding-id> [reason]

dk-pr-review · 2026-08-27T18:39:48Z

-        instruments.abortedRequests.add(1, { [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName })
+        instruments.abortedRequests.add(1, {
+          [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName),
+        })


[Functional.Interface] 🟡 RESTRICT

The handler="undefined" fallback is also applied to the OpenTelemetry instruments, but the justification documented in requestHandlerLabel.ts (Node cluster IPC serializes the prom-client worker registry as JSON and drops undefined values) only holds for the prom-client cluster aggregation path. OTel instruments export per-process and never traverse the cluster IPC JSON round-trip, so this changes the attribute set — and therefore the time-series identity — of the existing diagnostics abortedRequests/responseSizes/totalRequests/requestTimings series from "handler attribute absent" to handler="undefined". The CHANGELOG documents the change only for runtime_http_*, so consumers of the OTel/diagnostics metrics get an undocumented breaking change to their dashboards and alerts at the 7.5.0 boundary.

Action: Either state explicitly in the CHANGELOG that the diagnostics/OTel handler attribute also changes from absent to undefined (and why consistency with the Prometheus series is desired), or keep the OTel call sites unchanged if the diagnostics backend already renders the missing attribute in a way existing dashboards depend on.

To dismiss: /dk-review dismiss 3f2b6c1a-9d47-4e58-b1c2-7a0d5e83f914 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+
+import { requestHandlerLabel, UNNAMED_REQUEST_HANDLER } from '../requestHandlerLabel'
+import { addRequestMetricsMiddleware } from '../requestMetricsMiddleware'
+


[Functional.Check] 🔵 SUGGEST

The new regression suite only exercises addRequestMetricsMiddleware (prom-client). otelRequestMetricsMiddleware.ts received the same four-call-site change in this PR and remains completely untested, so a future revert or a missed call site there would not be caught by CI.

Action: Add an equivalent test for addOtelRequestMetricsMiddleware with stubbed instruments, asserting that RequestsMetricLabels.REQUEST_HANDLER is always a non-empty string for aborted, sized, counted and timed requests.

To dismiss: /dk-review dismiss b7c4e290-15af-4c63-8f0d-2e6a9d31c085 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+
+    handlerLabelledMetrics.forEach((metric) => {
+      samplesOf(aggregated, metric).forEach((sample) => {
+        expect(sample).toMatch(/handler="[^"]+"/)


[Evolvability.SolutionApproach] 🔵 SUGGEST

The emits no sample with a missing or empty handler label test asserts inside a nested forEach, so it passes vacuously whenever samplesOf returns an empty array — e.g. if a metric is renamed, if the histogram never observes (response length falsy, close never emitted), or if register.clear() wipes an instrument the test expected. A regression that stops emitting these series altogether would be reported as green.

Action: Assert expect(samples.length).toBeGreaterThan(0) for each metric before iterating, so the test fails when the expected samples are absent rather than silently passing.

To dismiss: /dk-review dismiss 6d9a8e33-42b7-4b1e-9c5f-8b21f047ad6e [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+// Node's cluster IPC serializes messages as JSON, which drops properties whose
+// value is `undefined`. This is what the master receives from each worker.
+const overClusterIpc = <T>(payload: T): T => JSON.parse(JSON.stringify(payload))
+


[Evolvability.Organizational] 🔵 SUGGEST

overClusterIpc is defined identically (implementation plus explanatory comment) in both requestHandlerLabel.test.ts and clusterMetricsAggregator.test.ts. Since it encodes a non-obvious invariant about the cluster IPC JSON round-trip that both suites depend on, duplicating it means a future correction has to be found and applied twice.

Action: Extract overClusterIpc (with its comment) into a shared test helper under src/service/metrics/__tests__/ and import it from both suites.

To dismiss: /dk-review dismiss 0a5f7c18-3e6d-4a92-bb47-5c9e2d10f8a3 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+// Closing the response inside `next()` makes the middleware finish its timings
+// synchronously, so no stream plumbing is needed.
+const runRequest = async (middleware: any, ctx: any) => {
+  await middleware(ctx, async () => {


[Evolvability.SupportedByLanguage] 🔵 SUGGEST

runRequest(middleware: any, ctx: any) and the untyped object returned by buildCtx opt the whole suite out of type checking against ServiceContext and the middleware signature. The tests are specifically about a context property (ctx.requestHandlerName), so if that property is renamed or the middleware signature changes, these tests will compile cleanly and fail only at runtime — or, worse, keep passing against a stale shape.

Action: Type buildCtx as Partial<ServiceContext> cast once at the boundary and give runRequest the real middleware type ((ctx: ServiceContext, next: () => Promise<void>) => Promise<void>), keeping the any cast confined to the single stub construction.

To dismiss: /dk-review dismiss e21c9b46-77d0-4f35-a8e3-1b6f4c9d2057 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+    }
+
+    await statusTrackHandler(ctx as ServiceContext)
+


[Functional.Resource] 🔴 BLOCK

The new test invokes statusTrackHandler without stubbing process.send. LINKED is !!process.env.VTEX_APP_LINK, which is false under Jest, so the handler executes process.send?.('broadcastStatusTrack') for real. When Jest runs this file in a child-process worker (the default whenever more than one test file runs, including yarn ci:test), process.send is the jest-worker IPC channel; jest-worker's parent _onMessage switches on response[0] and throws TypeError: Unexpected response from worker: b for an unrecognized string message, aborting the run. Both tests in the file trigger this.

Action: Stub the IPC channel in the test, following the convention already used in src/service/metrics/__tests__/clusterMetricsAggregator.test.ts: set (process as any).send = jest.fn() in beforeEach and delete (process as any).send in afterEach. Assert the broadcast while you are there — expect(sendMock).toHaveBeenCalledWith('broadcastStatusTrack') — so the side effect is covered rather than merely leaked.

To dismiss: /dk-review dismiss 3f2b8c41-9d6e-4a17-b0c5-7e21a4f8d093 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+
+    handlerLabelledMetrics.forEach((metric) => {
+      samplesOf(aggregated, metric).forEach((sample) => {
+        expect(sample).toMatch(/handler="[^"]+"/)


[Functional.Check] 🟡 RESTRICT

The assertion in 'emits no sample with a missing or empty handler label' is vacuous: samplesOf(aggregated, metric).forEach(...) runs zero assertions when a metric produces no samples, so the test passes if the fix regresses to the point where the metric disappears from the aggregated output altogether. That is precisely the failure mode this PR is guarding against (a label/series vanishing through the cluster IPC round-trip).

Action: Assert the sample set is non-empty before iterating, e.g. const samples = samplesOf(aggregated, metric); expect(samples.length).toBeGreaterThan(0); samples.forEach(...), or use expect.hasAssertions() plus an explicit expected-series list per metric.

To dismiss: /dk-review dismiss b7e14d02-5a38-4c96-8f21-6d09c3ba7e58 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

      if (instruments) {
-        instruments.abortedRequests.add(1, { [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName })
+        instruments.abortedRequests.add(1, {
+          [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName),


[quality.new-logic-enforcement] 🔵 SUGGEST

otelRequestMetricsMiddleware.ts gains the same requestHandlerLabel(...) fallback at four instrument call sites (abortedRequests, responseSizes, totalRequests, requestTimings) but no test accompanies it. The new requestHandlerLabel.test.ts covers only the prom-client path via addRequestMetricsMiddleware; the OTel middleware's attribute handling stays uncovered, so a future call site added without the helper would not be caught.

Action: Add a test for addOtelRequestMetricsMiddleware with a mocked getOtelInstruments that asserts every recorded attribute set carries handler: 'undefined' when ctx.requestHandlerName is unset, mirroring the prom-client tests.

To dismiss: /dk-review dismiss c94a6f37-2b81-40de-9a53-18f7ce20b4d6 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+// Minimal ServiceContext stand-in for addRequestMetricsMiddleware: it only needs
+// `req`/`res` emitters and a `response` with `length` and `status`.
+const buildCtx = (requestHandlerName?: string) => {
+  const res = new EventEmitter()


[quality.new-logic-enforcement] 🔵 SUGGEST

buildCtx fixes requestHandlerName at context-construction time, so every test reads a handler name that already exists before the middleware runs. Real code — including statusTrack.ts in this very PR — assigns ctx.requestHandlerName during next(), and the middleware reads it afterwards in its finally block. That late-assignment ordering, the behaviour the statusTrack change depends on, is never exercised.

Action: Add a case where next() sets ctx.requestHandlerName = 'builtin:status-track' before emitting close, and assert the emitted series is handler="builtin:status-track" rather than handler="undefined".

To dismiss: /dk-review dismiss e50c8a19-7f43-4b2a-93d8-4c6e1b57a082 [reason]

dk-pr-review · 2026-08-27T18:39:48Z

+// values, which used to strip the `handler` label from the aggregated output).
+const overClusterIpc = <T>(payload: T): T => JSON.parse(JSON.stringify(payload))
+
 const aggregateRegistries = async (registries: Array<Registry>): Promise<string> => {


[Evolvability.Organizational] 🔵 SUGGEST

overClusterIpc is defined verbatim in two test files (clusterMetricsAggregator.test.ts line 55 and requestHandlerLabel.test.ts line 9), each with its own copy of the explanatory comment. Duplicated test infrastructure drifts: the two comments already differ in wording, and a future correction to the IPC-fidelity model has to be applied twice.

Action: Extract the helper into a shared test utility (e.g. src/service/metrics/__tests__/helpers/overClusterIpc.ts) and import it from both files, keeping a single authoritative comment.

To dismiss: /dk-review dismiss a2d76b58-8c14-4e93-b7a0-5f39284ce671 [reason]

silvadenisaraujo added 3 commits August 3, 2026 14:39

chore(changelog): document handler label fixes

9581066

silvadenisaraujo self-assigned this Aug 3, 2026

silvadenisaraujo mentioned this pull request Aug 3, 2026

Backport cluster-wide prom-client metrics aggregation (plus undefined-handler-label fix) to 6.x #675

Merged

12 tasks

caroolcanelas2 approved these changes Aug 4, 2026

View reviewed changes

juliobguedes approved these changes Aug 4, 2026

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix/metrics-undefin…

5272de5

…ed-handler-label

dk-pr-review Bot reviewed Aug 27, 2026

View reviewed changes

bump version and consolidate changelog

2924d2f

dk-pr-review Bot reviewed Aug 27, 2026

View reviewed changes

juliobguedes merged commit 2758c5f into master Aug 27, 2026
40 checks passed

juliobguedes deleted the fix/metrics-undefined-handler-label branch August 27, 2026 18:47


		import { requestHandlerLabel, UNNAMED_REQUEST_HANDLER } from '../requestHandlerLabel'
		import { addRequestMetricsMiddleware } from '../requestMetricsMiddleware'

Uh oh!

Conversation

silvadenisaraujo commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Which requests are affected

Proposal

Tests

Rollout note

Uh oh!

juliobguedes commented Aug 27, 2026

Uh oh!

dk-pr-review Bot left a comment

Choose a reason for hiding this comment

DK Review — Audit Summary

BLOCK

RESTRICT

SUGGEST

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

sonar-workflows Bot commented Aug 27, 2026

Quality Gate passed

Issues

Measures

Uh oh!

dk-pr-review Bot left a comment

Choose a reason for hiding this comment

DK Review — Audit Summary

BLOCK

RESTRICT

SUGGEST

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dk-pr-review Bot Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

silvadenisaraujo commented Aug 3, 2026 •

edited

Loading